Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature "consolidated updates" #63

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jvanasco
Copy link
Contributor

This was fun to fix.

pyramid_redis_sessions was communicating with Redis WAY TOO MUCH. It set the TTL every single time an attribute was touched. Same thing with SET.

This fixes that!

The @persist and @refresh decorators now simply mark the _session_state as needing to persist or refresh.

The actual work is carried out by add_finished_callback (which always runs). it could be changed to add_response_callback. That will require a simple find/replace AND updating the args to _finished_callback.

If you want to immediately persist, the session now has do_persist and do_refresh methods. The callback function actually calls these too.

In order to make tests pass, the sessions must be told to inst.do_persist().

This patch cuts down on traffic A LOT. like a lot a lot.

Updated, documented issue [https://github.com//pull/63]

@jgelens
Copy link

jgelens commented Nov 19, 2015

setex is deprecated, just use .set(key, value, ex=expire_time)

@jvanasco
Copy link
Contributor Author

SET requires the server to be at least 2.6.12

That seems rather recent and likely to break systems or require upgrade.

On Nov 19, 2015, at 2:55 AM, Jeffrey Gelens [email protected] wrote:

setex is deprecated, just use .set(key, value, ex=expire_time)


Reply to this email directly or view it on GitHub.

@ericrasmussen
Copy link
Owner

FYI -- I've been out for the week but will be back soon and will start responding. Thanks for all of the work on these!

@jvanasco
Copy link
Contributor Author

Glad to help. Hope this is useful, it's been taking a load off my server. And thanks for writing a package that is easy to read and modify. Some of the other session packages.... grrr..

I'm gonna be offline for most of the next week so I just wanted to note that 'consolidated_updates' means "consolidating the expiration updates of existing redis keys" - it's not a consolidation of all the tickets/prs.

I wanted to work on some of the auto-expiry stuff too that someone mentioned on another ticket but I didn't think it is worth it yet. Redis doesn't currently have a command that can run a single lookup for a key+value, so there would need to be 2 O(1) queries and lookups per session hit to support that -- and possibly another to update. The overhead of 2 guaranteed gets + 1 potential set isn't worth the utility of that feature. I've logged a feature request to Redis to do everything at once, and am lobbying their devs.

If Redis will support a consolidated GET for value + expiry, the next feature I'd target is to add a configurable "float" for EXPIRE renewals. The general idea is that if you have a 1200s expiry window and a 10s float, EXPIRE is only renewed if the session is more than 1190s old. In practice, if a page loaded a total of 10 ajax calls and assets locked behind a session, the float would result in only 1 expire command -- the package just wouldn't bother to hit redis for the other 9 (or subsequent pageviews for a bit).

@wichert
Copy link

wichert commented May 6, 2016

@ericrasmussen Is still still on your radar?

@jvanasco
Copy link
Contributor Author

jvanasco commented May 6, 2016

FYI, I've been using this branch in production - https://github.com/jvanasco/pyramid_redis_sessions/tree/custom_deployment

It has a handful of other fixes and improvements, including declaring redis as an LRU cache so no SETEX/GETEX happens in python.

@ericrasmussen
Copy link
Owner

@wichert it is but there are some extenuating circumstances and I don't know when I will be able to get back to it. If someone wants to start a discussion in #pyramid on freenode we can talk about moving the project over if anyone else would like to take over for now.

@digitalresistor
Copy link
Collaborator

@ericrasmussen Since you asked on IRC. No, this does not break any contracts with pyramid sessions.

Cookie based sessions for example are not "committed" until the user actually receives the updated cookie, which is done in an add_finished_callback. There is no requirement that a session implementation immediately persists data to its backend.

@jvanasco
Copy link
Contributor Author

FYI, I'm now using a branch called 'custom_development' that fixes additional bugs and design issues in the original package. I would suggest looking at that for merges instead of this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants